👍 Ah, ok, so this is fine because we erase regions in check_ptr_ptr_cast.

This will only work for pointers that can be coerced modulo regions, it will still let something like this pass:

trait Trait<'a> {}

fn cast<'a, 'b>(x: *mut dyn Trait<'a>) -> *mut (dyn Trait<'b> + Send) {
    x as _
}

I think the correct fix would be to actually borrow check the principals for equality around here:

Also, extending the trait object lifetime itself (e.g. *mut dyn Trait<'a> + 'a -> *mut dyn Trait<'a> + 'static) is harmless and should be allowed, just like adding auto-traits is.

Edit: borrowck version here: c471b9b

something like this pass:

It does indeed let it pass, but why? These are not coercible?..

I think the correct fix would be to actually borrow check the principals for equality around here:

I also thought of that, but it seemed somewhat painful to do. And since we handle upcasts with CastKind::PointerCoercion, this seemed like a good solution. Borrowck change might be the right solution after all though.

Edit: borrowck version here: c471b9b

This won't help with *mut (u8, dyn Trait<'a>) -> *mut (u8, dyn Trait<'b>), but that should be an easy fix with struct_tail...

Also, extending the trait object lifetime itself (e.g. *mut dyn Trait<'a> + 'a -> *mut dyn Trait<'a> + 'static) is harmless and should be allowed, [...]

Hm, I can't find an example why it isn't harmless, but I also can't really convince myself that it is. Is this something to do with the fact that you can given dyn Trait<'a> + 'b you can use 'a in Trait's method's signatures, but not 'b?

[...], harmless and should be allowed, just like adding auto-traits is.

Adding auto-traits is not harmless. Here is an example that segfaults in safe code with arbitraty self receivers and adding send:

#![feature(arbitrary_self_types)]

trait Trait {
    fn f(self: *const Self)
    where
        Self: Send;
}

impl Trait for *const () {
    fn f(self: *const Self) {
        unreachable!()
    }
}

fn main() {
    let unsend: *const dyn Trait = &(&() as *const ());
    let send_bad: *const (dyn Trait + Send) = unsend as _;
    send_bad.f();
}

fish: Job 1, './t' terminated by signal SIGSEGV (Address boundary error)

Notably, even though we had to implement f, it's not actually in a vtable, since you should not be able to call it (because of unsatisfied Send bound):

error: vtable entries for `<*const () as Trait>`: [
           MetadataDropInPlace,
           MetadataSize,
           MetadataAlign,
           Vacant,
       ]
  --> ./t.rs:24:1
   |
24 | trait Trait {
   | ^^^^^^^^^^^

Adding auto-traits is not harmless. Here is an example that segfaults in safe code with arbitraty self receivers and adding send:

Interesting. I was somehow convinced these functions were still codegened and thought it'd be harmless because you can't do anything bad with with a sendable raw pointer alone.

This also means that borrow check alone won't be enough and we need to treat these casts more like coercions.

Is this something to do with the fact that you can given dyn Trait<'a> + 'b you can use 'a in Trait's method's signatures, but not 'b?

Yeah that was my reasoning as well. You can add bounds like Self: 'static, but since the maybe-not-'static thing is behind a raw pointer you shouldn't be able to do anything bad with it. But that same reasoning was already wrong for auto traits so I'm not sure anymore.

Also, extending the lifetime of trait objects is often used in unsafe code (like the one in std), so forbidding this will probably cause widespread breakage.

something like this pass:

It does indeed let it pass, but why? These are not coercible?..

What I'm trying to say is that because these are not coercible, the try_coercion_cast above will fail, which means that the cast will be a CastKind::PtrToPtr and not a coercion. And CastKind::PtrToPtr is absolutely not borrow checked so any lifetime can be cast to any other lifetime.

This also means that borrow check alone won't be enough and we need to treat these casts more like coercions.

I think that we can check that in hir_typeck, the same way we check

let x: &dyn Trait + Send = &();
let y: &dyn Trait = x;

(I think this is done in hir_typeck? will have to find out...)

What I'm trying to say is that because these are not coercible, the try_coercion_cast above will fail, which means that the cast will be a CastKind::PtrToPtr and not a coercion. And CastKind::PtrToPtr is absolutely not borrow checked so any lifetime can be cast to any other lifetime.

Ah, I see. Right.

Do you think moving the check out of try_coercion_cast's match would fix the issue (possibly always reporting an error if try_coercion_cast fails for *mut dyn ...)? Or should I move to your proposed solution with doing things for PtrToPtr in borrowck?

Do you think moving the check out of try_coercion_cast's match would fix the issue (possibly always reporting an error if try_coercion_cast fails for *mut dyn ...)?

Where exactly do you want to move it? If this is checked unconditionally even after do_check, then all ptr-ptr casts with vtables that can't be coerced will be just be forbidden, right? Like casting *mut Wrapper<dyn Trait> <-> *mut dyn Trait.

I think that we can check that in hir_typeck, the same way we check

let x: &dyn Trait + Send = &();
let y: &dyn Trait = x;

I think checking this here in check_ptr_ptr_cast where we also compare the principals might be fine. Just check that the source includes all auto traits from the target.

And I'm pretty sure we'd still need borrowck for the principal on top of that.

Checking this with the Unsize looks risky to me. That trait is also implemented for trait upcasts, e.g. dyn Subtrait: Unsize<dyn Supertrait>, because trait upcasting is essentially treated as a "unsizing" coercion before codegen, except that the source type is already unsized.

Because of that, with your current implementation this will compile:

trait Super {}
trait Sub: Super {}

struct Wrapper<T: ?Sized>(T);

fn cast(ptr: *const dyn Sub) -> *const Wrapper<dyn Super> {
    // this is a "reinterpret cast" and not an upcast, the vtable is just copied
    ptr as _
}

This could theoretically be fine, because it looks like there is currently no (safe) way to cast from *const Wrapper<dyn Super> to *const dyn Super. This happens, because try_coercion_cast will (via Coerce::coerce_unsized) eagerly evaluate the CoerceUnsized and Unsize obligations, but not their nested obligations, which means that casting *const Wrapper<dyn Super> to *const dyn Super is treated as a coercion and fails. But this also leads to weird inconsistencies whether a cast is considered a coercion cast or pointer-pointer cast.

trait Trait {}

struct Wrapper<T: ?Sized>(T);

fn cast1(x: *const dyn Send) -> *const (dyn Send + Sync) {
    x as _ // is a ptr-ptr cast, compiles
}

fn cast2(x: *const Wrapper<dyn Send>) -> *const (dyn Send + Sync) {
    x as _ // is an unsizing coercion cast, error
}

So I'd argue the current behavior is a bug and it should always be allowed to cast *const Wrapper<dyn Trait> to *const dyn Trait, which would make your implementation incorrect.

*const dyn Sub -> *const Wrapper<dyn Super> being a "reinterpret cast" is very bad, for the same reasons we have the issues this PR attempts to fix — having an incorrect vtable is not sound with other features. I'll add back the check that the principal traits are the same. While I don't see a safe way to go *const W<dyn Trait> -> *const dyn Trait, this transformation looks more than fine for me, so I really don't want to depend on it being impossible. (tbh I assumed it was possible, considering you can do *const dyn Trait -> *const W<dyn Trait>, it's very weird we don't allow the opposite...)

So I'd argue the current behavior is a bug and it should always be allowed to cast *const Wrapper<dyn Trait> to *const dyn Trait, which would make your implementation incorrect.

In other words: I agree with this.

Although I'm not sure how you can add this in a non-confusing way.

• *const () -> *const Send must be unsizing
• *const W<dyn Trait> -> *const dyn Trait should be ptr-ptr
  • But what if W<dyn Trait>: Trait? Currently it's unsizing, do we keep that?

• But what if W<dyn Trait>: Trait? Currently it's unsizing, do we keep that?

*const W<dyn Trait> -> *const dyn Trait can't be an unsizing cast, because W<dyn Trait> is not sized and not a trait object. We can't keep the vtable metadata that is already in the fat pointer *const W<dyn Trait> and add the metadata of an impl Trait for W<dyn Trait> on top of that.

Casting *const T to *const U should be an unsizing coercion if and only if T: Unsize<U>, but W<dyn Trait> can never implement Unsize<dyn Trait>.

The problem with the current implementation is that it doesn't fully check W<dyn Trait>: Unsize<dyn Trait>. It only sees that W<dyn Trait>: Unsize<dyn Trait> holds if the nested obligations W<dyn Trait>: Sized and W<dyn Trait>: Trait hold but then directly assumes the cast must be an unsizing coercion and only checks the nested obligations too late.

Oooh, right.

Yeah, then *const W<dyn Trait> -> *const dyn Trait should definitely be supported, but maybe as a follow up to this :)

It is possible to do *const W<dyn Trait> -> *const dyn Trait right now in safe code. You just need to write a small helper function

impl<T: ?Sized> Wrapper<T> {
    fn get(this: *const Self) -> *const T {
        this as _
    }
}

fn cast(ptr: *const Wrapper<dyn Super>) -> *const dyn Super {
    Wrapper::get(ptr)
}